Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RTG] Add ISA assembly emission pass #8057

Open
wants to merge 1 commit into
base: maerhart-rtgtest-instructions
Choose a base branch
from

Conversation

maerhart
Copy link
Member

@maerhart maerhart commented Jan 9, 2025

No description provided.

@maerhart maerhart added the RTG Involving the `rtg` dialect label Jan 9, 2025
@maerhart maerhart requested a review from darthscsi January 9, 2025 21:48
@maerhart maerhart force-pushed the maerhart-rtg-emit-assembly branch from 44b5801 to babe1f2 Compare January 9, 2025 22:01
@maerhart maerhart force-pushed the maerhart-rtgtest-instructions branch from afd891b to fc18855 Compare January 21, 2025 10:48
@maerhart maerhart force-pushed the maerhart-rtg-emit-assembly branch from babe1f2 to 8ed5c6e Compare January 21, 2025 11:02
Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo the StringSet.


LogicalResult emitInstruction(InstructionOpInterface instr) {
os << llvm::indent(4);
bool useBinary = unsupportedInstr.contains(instr->getName().getStringRef());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name internally is stored as an Attribute. A set over StringAttr would likely be more efficient than a StringSet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which obviously requires constructing StringAttrs for each unsupoorted instruction at initialization time, but that's a one time cost.

llvm::raw_ostream &os;

/// Instructions to emit in binary.
const llvm::StringSet<llvm::MallocAllocator> &unsupportedInstr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, a DenseSet is probably much faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RTG Involving the `rtg` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants